More flexible --test option #359

mattias-p · 2023-11-23T08:16:31Z

Purpose

This PR implements flexible command line overriding of which test cases to run from the command line.

Context

This is an extension to the improvements in Extend --test option to allow passing only a testcase #333.
Replaces Better performance when running specified test. Function to exclude tests #130. The point of that PR is to allow users to exclude test cases from the command line. This PR includes the same functionality using a different command line syntax.
Leverages Ability to run single testcase via test_zone() zonemaster-engine#1312 to properly handle the Basic test module (c.f. Better performance when running specified test. Function to exclude tests #130 (comment)).
Handles the interaction of --test and --profile in accordance with Better performance when running specified test. Function to exclude tests #130 (comment).
For what it's worth, the idea for this design came to me in a discussion about Refactoring of function Zonemaster::Engine::Test::run_one() zonemaster-engine#1294.

Changes

The set of values accepted by --test is extended.
The semantics are changed of repeated --test options. They used to be cumulative but now the last one takes precedence.
Which test cases to include in the run is now controlled by setting the test_cases profile property and running Zonemaster::Engine->test_zone().
When --test=basic or --test=basic02 is specified, and basic02 considers the domain too broken to test, a CANNOT_CONTINUE message is now emitted, whereas it wasn't emitted before this PR.

Reviewing

All updates for this PR are contained in the very last commit. The commits before the last one all belong to Organize --help text #389 and should be disregarded when reviewing this PR.
Look especially for lacking test coverage.

Testing

Unit tests should pass.

matsduf · 2023-11-23T08:34:02Z

Before this PR CLI would call either test_zone, test_module or test_method in Zonemaster::Engine
to run the test. After this PR CLI always calls test_zone and uses the test_cases profile property to
control which tests are run. This means that the Basic tests are always included even when a single test case
is specified.

Since the semantics of specifying a single test case or single test module is changed so that the Basic tests
are included, an argument could be made that this is a breaking change, but I believe an argument could also
be made that it's still just an addition.

That is a bad change. When working with test zones you want to be able to run just the specific test case and nothing else.

Secondly, with that change, if Basic fails completely no other test case is run, and that is not what we want when testing and creating test zones.

I am against a change that makes it impossible to run just the selected test case.

mattias-p · 2023-11-23T09:05:36Z

All I'm hearing is "no" and "bad". Is there anything about this proposal that you do like?

Also, I'm trying to understand your objection here. AFAICT you're saying that when a user requests that a single test case is run on a domain that doesn't fulfill the requirements posed by Basic, the requested test case still needs to run. Is that correct? And if so, could you explain why this is important, because it's not obvious to me.

matsduf · 2023-11-23T12:20:50Z

All I'm hearing is "no" and "bad". Is there anything about this proposal that you do like?

It would be good to be able to exclude test cases without creating a new profile. It would be good to be able to select two or more test cases and they are run without overlap.

Sometimes it is probably good that Basic is run before a selected test.

Also, I'm trying to understand your objection here. AFAICT you're saying that when a user requests that a single test case is run on a domain that doesn't fulfill the requirements posed by Basic, the requested test case still needs to run. Is that correct? And if so, could you explain why this is important, because it's not obvious to me.

I want to see what happens in that test case for the test zone even when there are errors. And I want minimal output without the extra output from Basic when testing test zones. Sometimes I have to run with --level debug which gives many lines, which will be anymore if Basic is run.

Give the possibility to run a test without adding Basic.

If Basic is not in the profile, why should Basic still be run? The magic could rather be to put the Basic test cases in the profile first, if there are any.

mattias-p · 2023-11-23T15:19:11Z

I want to see what happens in that test case for the test zone even when there are errors. And I want minimal output without the extra output from Basic when testing test zones. Sometimes I have to run with --level debug which gives many lines, which will be anymore if Basic is run.

If Basic is not in the profile, why should Basic still be run? The magic could rather be to put the Basic test cases in the profile first, if there are any.

Ok. Yeah, I see how this could be very valuable in development contexts. And I also don't particularly like that Basic is forced.

I have an idea of how we could simplify things in Engine in a way that supports this. I think I'll make a draft PR there too.

I won't make any more updates to the code here before Engine is in a better shape to support it. But if anyone has more feedback on the direction of this PR, please let me know. Maybe it'll affect the updates in Engine.

matsduf · 2024-11-11T09:36:19Z

@mattias-p, is this still relevant or can it be closed now?

mattias-p · 2024-11-11T13:48:24Z

This conflicts with #371 as well as some of its documentation improvement followups. I plan to rebase this PR once those things have been merged. It seems somewhat unreasonable to get this merged for 2024.2 so I postponed it.

mattias-p · 2024-11-15T09:49:11Z

I rebased this onto latest develop and fixed some typos added some unit tests. I also tightened some validations for sanity reasons, but that shouldn't have any effect on zonemaster-cli behavior.

matsduf · 2024-11-15T12:01:18Z

This is not true anymore. Please update description.

* This PR contains the commits from [Organize --help text #389](https://github.com/zonemaster/zonemaster-cli/pull/389), which should be merged before this one.

matsduf · 2024-11-15T12:31:28Z

Do --test=-dnssec and --test=all-dnssec mean the same thing?

---> No

Do --test= and --test=all mean the same thing?

---> No

matsduf · 2024-11-15T16:25:00Z

script/zonemaster-cli

+
+=item --test=B<nameserver>
+
+runs the entire Nameserver test module, and nothing else.


Suggested change

runs the entire Nameserver test module, and nothing else.

runs the entire Nameserver test module, including any test cases

disabled by the profile, but nothing else.

I agree that the documentation should make this clear, but I don't think it needs to be repeated for every example. I'll add a separate paragraph clarifying how it interacts with other options.

That could be fine too.

script/zonemaster-cli

matsduf · 2024-11-15T16:28:40Z

script/zonemaster-cli

+runs the entire DNSSEC test module except the DNSSEC05 test case, and nothing
+else.


Suggested change

runs the entire DNSSEC test module except the DNSSEC05 test case, and nothing

else.

runs the entire DNSSEC test module including any test cases

disabled by the profile, except the DNSSEC05 test case,

and nothing else.

script/zonemaster-cli

matsduf · 2024-11-15T16:31:12Z

script/zonemaster-cli

@@ -114,14 +114,50 @@ Print the effective profile used in JSON format, then exit.
 =item B<--test>=TESTCASE, B<--test>=TESTMODULE


Suggested change

=item B<--test>=TESTCASE, B<--test>=TESTMODULE

=item B<--test>=TESTCASE, B<--test>=TESTMODULE, B<--test>=EXPR

matsduf · 2024-11-15T16:34:19Z

script/zonemaster-cli

+=item --test=B<-dnssec>
+
+removes all test cases of the DNSSEC module from the set of tests that would
+otherwise have run.


Suggested change

otherwise have run.

otherwise have run, i.e. the test cases enabled by the profile.

matsduf · 2024-11-15T16:34:44Z

script/zonemaster-cli

+=item --test=B<+syntax01+syntax02>
+
+adds the Syntax01 and Syntax02 test cases to the set of tests that would
+otherwise have run.


Suggested change

otherwise have run.

otherwise have run, i.e. the test cases enabled by the profile.

matsduf · 2024-11-15T16:37:43Z

script/zonemaster-cli

+runs all test cases of all test modules except for the Nameserver test module in
+which only the Nameserver03 test case is run.


Suggested change

runs all test cases of all test modules except for the Nameserver test module in

which only the Nameserver03 test case is run.

runs all defined test cases of all defined test modules (disregardning the profile)

except for the Nameserver test module in which only the Nameserver03 test

case is run.

matsduf · 2024-11-15T16:42:20Z

Nice to have it consistent. Nice to be able to control what test cases to run without creating a custom profile.

mattias-p · 2024-11-15T17:14:03Z

Shouldn't it be --test=EXPR?

Fixing.

Do --test=-dnssec and --test=all-dnssec mean the same thing?

---> No

Do --test= and --test=all mean the same thing?

---> No

Agreed. In case you modify the test_cases property using --profile they won't be the same.

Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>

mattias-p · 2024-11-15T17:35:48Z

I think the current implementation is flawed regarding the interpretation of this command sequence:

printf "%s\n" "--test=-delegation01" > ~/.zonemaster/cli.args
zonemaster-cli example.com
zonemaster-cli --test=-delegation02 example.com

With the current proposal, in the first command delegation01 would be disabled, but in the second command it would be enabled again. This is because the second command would effectively be zonemaster-cli --test=-delegation01 --test=-delegation02 example.com and the second --test would override the first one. It would make much more sense if both --test arguments were applied as they appear on the command line. zonemaster-cli --test=-delegation01 --test=basic01 would still mean execute basic01 and nothing else.

matsduf · 2024-11-15T19:38:38Z

I think the current implementation is flawed regarding the interpretation of this command sequence:
printf "%s\n" "--test=-delegation01" > ~/.zonemaster/cli.args
zonemaster-cli example.com
zonemaster-cli --test=-delegation02 example.com
With the current proposal, in the first command delegation01 would be disabled, but in the second command it would be enabled again. This is because the second command would effectively be zonemaster-cli --test=-delegation01 --test=-delegation02 example.com and the second --test would override the first one. It would make much more sense if both --test arguments were applied as they appear on the command line. zonemaster-cli --test=-delegation01 --test=basic01 would still mean execute basic01 and nothing else.

I think it is correct that --test on the command line overrides --test in the configuration file. That is how it works in other cases, e.g. --level=debugon the command line overrides --level=info in the configuration file.

mattias-p · 2024-11-15T21:34:34Z

We have two kinds of option today: overwriting ones and aggregating ones. Most of our options overwrite, but --test, --ns and --ds aggregate. I'm arguing that it makes more sense to keep --test as an aggregating option than to change it into an overwriting one. I.e., --test=basic01 --test=+basic02 should be equivalent to --test=basic01+basic02, not to --test=+basic02.

mattias-p · 2024-11-15T22:38:02Z

I pushed an update makes --test aggregate again. It's in a separate commit in case you want to play with both the overwriting version and the aggregating one. The man page needs an update to clarify the semantics in of repeated --test options but I haven't got around to that.

I haven't forgotten your unresolved review comments, @matsduf. It's just that before I take care of them I want to settle this overwriting/aggregating business.

matsduf · 2024-11-18T12:30:01Z

We have two kinds of option today: overwriting ones and aggregating ones. Most of our options overwrite, but --test, --ns and --ds aggregate. I'm arguing that it makes more sense to keep --test as an aggregating option than to change it into an overwriting one. I.e., --test=basic01 --test=+basic02 should be equivalent to --test=basic01+basic02, not to --test=+basic02.

There is a clear difference between --ns and --ds, on one hand, and --test, on the other hand.

Options --ns and --ds are typically used for a specific zone. It is unlikely that you would put that into the the configuration file cli.args. Therefor it works to have them aggregating. On the other hand, it would also work to change them so that only the last one is used, but so that you can specify multiple NS or DS, respectively, for the option, e.g. with a + sign or in quotemarks.

Option --test can be used in cli.args instead of creating a custom profile. Then it should be possible to override that with the settings on the command line.

matsduf · 2024-11-18T14:52:24Z

I guess the aggregate version could work too, but it is quite complex and requires updated documentation that clearly states how it should work.

cli.arg	command line	resulting
--test basic	--test consistency	Only consistency will be run
--test consistency	--test basic	Only basic will be run
--test basic	--test +consistency	basic and consistency will be run

Also on the command line, --test basid and --test consistency can be combined, but the order will govern the result. That must be stated.

mattias-p changed the base branch from master to develop November 23, 2023 08:16

mattias-p mentioned this pull request Nov 23, 2023

Extend --test option to allow passing only a testcase #333

Merged

matsduf mentioned this pull request Dec 4, 2023

Better performance when running specified test. Function to exclude tests #130

Closed

matsduf added this to the v2024.1 milestone Dec 4, 2023

mattias-p mentioned this pull request Dec 8, 2023

Ability to run single testcase via test_zone() zonemaster/zonemaster-engine#1312

Merged

mattias-p modified the milestones: v2024.1, v2024.2 Jun 17, 2024

mattias-p force-pushed the test-option branch from 87a3d76 to 3e3e077 Compare August 2, 2024 09:11

mattias-p modified the milestones: v2024.2, v2025.1 Nov 11, 2024

mattias-p force-pushed the test-option branch 2 times, most recently from ecf71b3 to b316e12 Compare November 12, 2024 17:25

matsduf modified the milestones: v2025.1, v2024.2 Nov 13, 2024

mattias-p force-pushed the test-option branch 2 times, most recently from 5ebbad5 to 7690a3c Compare November 14, 2024 14:02

mattias-p added the V-Major Versioning: The change gives an update of major in version. label Nov 14, 2024

mattias-p modified the milestones: v2025.1, v2024.2 Nov 14, 2024

mattias-p marked this pull request as ready for review November 14, 2024 15:15

Expression language for specifying tests to run

b536acf

mattias-p force-pushed the test-option branch from 7690a3c to b536acf Compare November 15, 2024 09:45

mattias-p requested review from matsduf, tolvmannen, tgreenx, marc-vanderwal and MichaelTimbert November 15, 2024 10:46

matsduf requested changes Nov 15, 2024

View reviewed changes

matsduf mentioned this pull request Nov 15, 2024

"--list-tests" should respect the selected profile and settings by "--test" #396

Open

mattias-p and others added 2 commits November 15, 2024 18:15

Update script/zonemaster-cli

ffcb541

Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>

Update script/zonemaster-cli

0276a57

Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>

mattias-p added 2 commits November 15, 2024 23:28

Typo

823a30f

Make --test aggregate again

54ec6aa

mattias-p modified the milestones: v2024.2, v2025.1 Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More flexible --test option #359

More flexible --test option #359

mattias-p commented Nov 23, 2023 •

edited

Loading

matsduf commented Nov 23, 2023 •

edited

Loading

mattias-p commented Nov 23, 2023

matsduf commented Nov 23, 2023

mattias-p commented Nov 23, 2023 •

edited

Loading

matsduf commented Nov 11, 2024

mattias-p commented Nov 11, 2024 •

edited

Loading

mattias-p commented Nov 15, 2024

matsduf commented Nov 15, 2024

matsduf commented Nov 15, 2024 •

edited

Loading

matsduf Nov 15, 2024

mattias-p Nov 15, 2024

matsduf Nov 15, 2024

matsduf Nov 15, 2024

matsduf Nov 15, 2024

matsduf Nov 15, 2024

matsduf Nov 15, 2024

matsduf Nov 15, 2024

matsduf commented Nov 15, 2024

mattias-p commented Nov 15, 2024

mattias-p commented Nov 15, 2024

matsduf commented Nov 15, 2024 •

edited

Loading

mattias-p commented Nov 15, 2024 •

edited

Loading

mattias-p commented Nov 15, 2024

matsduf commented Nov 18, 2024

matsduf commented Nov 18, 2024


		=item --test=B<nameserver>

		runs the entire Nameserver test module, and nothing else.

	runs the entire Nameserver test module, and nothing else.
	runs the entire Nameserver test module, including any test cases
	disabled by the profile, but nothing else.

		runs the entire DNSSEC test module except the DNSSEC05 test case, and nothing
		else.

		@@ -114,14 +114,50 @@ Print the effective profile used in JSON format, then exit.
		=item B<--test>=TESTCASE, B<--test>=TESTMODULE

	otherwise have run.
	otherwise have run, i.e. the test cases enabled by the profile.

		runs all test cases of all test modules except for the Nameserver test module in
		which only the Nameserver03 test case is run.

More flexible --test option #359

Are you sure you want to change the base?

More flexible --test option #359

Conversation

mattias-p commented Nov 23, 2023 • edited Loading

Purpose

Context

Changes

Reviewing

Testing

matsduf commented Nov 23, 2023 • edited Loading

mattias-p commented Nov 23, 2023

matsduf commented Nov 23, 2023

mattias-p commented Nov 23, 2023 • edited Loading

matsduf commented Nov 11, 2024

mattias-p commented Nov 11, 2024 • edited Loading

mattias-p commented Nov 15, 2024

matsduf commented Nov 15, 2024

matsduf commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Nov 15, 2024

mattias-p commented Nov 15, 2024

mattias-p commented Nov 15, 2024

matsduf commented Nov 15, 2024 • edited Loading

mattias-p commented Nov 15, 2024 • edited Loading

mattias-p commented Nov 15, 2024

matsduf commented Nov 18, 2024

matsduf commented Nov 18, 2024

mattias-p commented Nov 23, 2023 •

edited

Loading

matsduf commented Nov 23, 2023 •

edited

Loading

mattias-p commented Nov 23, 2023 •

edited

Loading

mattias-p commented Nov 11, 2024 •

edited

Loading

matsduf commented Nov 15, 2024 •

edited

Loading

matsduf commented Nov 15, 2024 •

edited

Loading

mattias-p commented Nov 15, 2024 •

edited

Loading